Include shared service instances in /v2/spaces/:guid/service_instances by jenspinney · Pull Request #961 · cloudfoundry/cloud_controller_ng

jenspinney · 2017-10-24T13:12:59Z

As an app dev (receiver), I can see information regarding service instances that have been shared with me. #152035378

What

This PR changes the /v2/space/:guid/service_instances endpoint such that it now includes any services instances that have been shared into the queried space. With this change, the following CLI commands now work with shared services:

cf service <shared-service-name>
cf bind-service
cf unbind-service

Exciting!

First Important Note

This PR may be a bit more controversial than some of our other recent PRs. We're very interested in your feedback and suggestions if you have ideas for how we could improve our approach. "Weirdness" in summary:

Imagine a user is querying /v2/spaces/:guid/service_instances for a space where a service instance has been shared into. Imagine also that this user does not have visibility permissions (i.e., space auditor or similar) for the source space of the shared service instance.
At the point at which the dataset to retrieve the service instances is actually executed against the database, we use the secure eager loader. In addition to loading the data for the main model being queried (i.e., service instance), the eager loader also attempts to fill in certain relationship information for the desired resource. In the case of service_instance, the service_instance.space relationship is also typically loaded. However, in this scenario, the user does not have visibility access to the service instance's space, so the space information is not loaded and service_instance.space will be null for this service instance. The code that does this is here. The default visibility filter calls the user_visibility_filter method on the model object, which for this example will return false.
The cloud controller code is generally written in a way that assumes service_instance.space is never nil. We are now breaking this assumption. We did an audit of all the places we could find that reference service_instance.space, and added nil guarding in every place were we could conceivably run into this scenario. This feels dirty and prone to edge-casey 500s in the future (i.e., if someone adds code later that assumes this value can never be nil and they don't test this narrow scenario). However, loading data that the user doesn't actually have permission to see feels dirty as well and would potentially require some messy surgery of very generic methods. In that case we also have the potential ability to accidentally reveal more information than the user is authorised to see. Despite the problems of letting service_instance.space be null, it seems like the better of two evils since this condition will only occur in a very narrow strip of code (i.e., #enumerate_service_instances in the v2 space controller) which will probably not be changed dramatically in the future, since it's v2.

Second Important Note

As part of doing this change, we also noticed the #read_permissions? implementation for service instance access seemed to be unnecessarily duplicating the behaviour and intent of #read?, but going through a very different codepath. By modifying #read_permissions? to use #read?, we believe we changed the code to actually be more correct by additionally allowing the global_auditor role to have access to this operation.

The #read_permissions access method is used to ultimately tell a service broker whether the active user has sufficient permission to view the service dashboard for a service instance. Based on our understanding of the global_auditor role, it seems to make sense that they would be allowed to view service dashboards. If our understanding is wrong, please let us know :)

Summary of Changes:

Backfilled missing unit tests for service_instance.user_visibility_filter function.
Modified user_visibility_filter on the ServiceInstance model to additionally allow user who have visibility to any space the service instance has been shared into.
Changed controller method for /v2/spaces/:guid/service_instance such that it returns all service instances either created in the given space or shared with the given space.
Guard against nil values for service_instance.space in service_instance_access.rb and in service_instance.rb.

PR

I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the master branch
I have run all the unit tests using bundle exec rake
I have run CF Acceptance Tests on bosh lite

Thanks, sapi (@jenspinney and @deniseyu)

cfdreddbot · 2017-10-24T13:13:00Z

Hey jenspinney!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

cf-gitbot · 2017-10-24T13:13:01Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/152238740

The labels on this github issue will be updated when the story is started.

cfdreddbot · 2017-10-24T13:13:01Z

Hey jenspinney!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

tcdowney · 2017-10-24T18:39:25Z

@zrob there are some product-facing questions in the description of this SAPI PR. Figured you'd want to weigh-in as well.

tcdowney · 2017-10-24T18:44:12Z


    def in_suspended_org?
-      space.in_suspended_org?
+      space && space.in_suspended_org?


@jenspinney @deniseyu

I'm not sure of all of the places where this is called, but I'm a bit worried that this will allow users to do things with the service instance when the org is suspended that they shouldn't be able to do.

E.g. Org One binds some email sending service to Space A and shares it with Space B in Org Two. If Org One is spamming people with their email sending service and is suspended will this mean that Org Two can keep on making changes to the service instance and using it since space will be nil and always return false?

Thanks for the feedback, @tcdowney!

This method appears to only be called in service_instance_access.rb.

As a general rule, if space is nil, the user may only do read-ish things (nothing involving modification, deletion, etc.) on the service instance because they don't have access to the originating space of the service instance. The places where we check whether the org is suspended are modification-type access checks, so we'll fail those anyway because space is nil. In our version of service_instance_access, you'll see that wherever this method is called, we've made changes to also explicitly check if the space is available.

If we reversed this and returned true for service_instance.in_suspended_org? when space is nil, it seems counterintuitive that it would be asserting that the org of the service instance is suspended even though we know nothing about the originating org. I would imagine if service_instance.in_suspended_org? returned true, I could return some error message like, "The org you're in has been suspended", which is not true in this case.

Thanks!
Jen and @deniseyu

Gerg · 2017-10-31T17:06:48Z

@tcdowney What were the product questions you wanted Zach to address?

@jenspinney I don't have strong feelings about the nil guarding for service_instance.space. The pair that reviews this next can check if there is a better way to encapsulate the the space maybe-not-being-there.

For the second important note, it looks like #792 introduced #read_permissions?. It might have been an oversight to not allow global auditors, but worth digging into.

Gerg · 2017-10-31T19:22:57Z

If you want to be extra-ruby, you can also use the new safe navigation operator. Instead of foo && foo.bar, you can now do foo&.bar.

jenspinney · 2017-11-01T14:28:19Z

Let's be extra ruby! Thanks for the info @Gerg :)

Updated the PR with &.'s.

And yes, we assumed the change to address #792 mistakenly left out global auditors. (If it was intentional though, we can unwind that change.)

Thanks,
SAPI team (Jen && @deniseyu)

tcdowney · 2017-11-01T23:42:42Z

@Gerg @jenspinney

And yes, we assumed the change to address #792 mistakenly left out global auditors. (If it was intentional though, we can unwind that change.)

That's the product question we had for @zrob. Sorry for not making that very clear.

Gerg · 2017-11-06T17:39:51Z

From our discussion with SAPI this morning/evening, it sounds like we want to break the global auditor permissions change into a separate PR.

[#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

[#152035378] Signed-off-by: Alex Blease <ablease@pivotal.io>

/v2/spaces/:guid/services endpoint now includes service instances that have been shared into the given space. [#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

* The only visible change is that the global auditor role now receives "true" for read when calling /v2/service_instances/:guid/permissions. This tells the service broker that the user is allowed to view the service instance dashboard. They already have read access to the service instance, so we don't anticipate that this is a problem. [#152035378] Signed-off-by: Derik Evangelista <devangelista@pivotal.io>

… spaces * Also cleaned up some of the null checking in service_instance_acess and added tests for access permissions on shared service instances. [#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

[#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

[#152631507] Signed-off-by: Denise Yu <dyu@pivotal.io>

deniseyu · 2017-11-07T14:25:44Z

We have rolled back the global auditors read-permissions. I've also rebased the branch against the latest upstream. Please let us know if you have further feedback!

matt-royal · 2017-11-13T19:10:43Z

+        [:shared_spaces, user.spaces_dataset],
+        [:shared_spaces, user.managed_spaces_dataset],
+        [:shared_spaces, user.audited_spaces_dataset],
+        [:shared_spaces, managed_organizations_spaces_dataset(user.managed_organizations_dataset)],


We dug into this method, and realized that it writes very inefficient SQL (involving many redundant subselects). Your PR didn't introduce this problem, it just exacerbated it by adding more subselects. We have a dedicated story to address this problem so this won't prevent us from merging this PR: https://www.pivotaltracker.com/story/show/152736102

Good spot! Thanks for making a separate story for it :)

cf-gitbot added the unscheduled label Oct 24, 2017

tcdowney reviewed Oct 24, 2017

View reviewed changes

tcdowney added the PM Review label Oct 25, 2017

Samze force-pushed the pr-service-instance-sharing-view-shared-service-in-space branch from 5d91a81 to af8f89e Compare October 31, 2017 15:45

deniseyu force-pushed the pr-service-instance-sharing-view-shared-service-in-space branch from af8f89e to 2da2072 Compare November 1, 2017 14:25

deniseyu mentioned this pull request Nov 1, 2017

Implement bare-bones GET /v3/service_instances #972

Merged

4 tasks

Alex Blease and others added 7 commits November 7, 2017 13:58

Add tests for ServiceInstance.user_visibility_filter

0974196

[#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

Look at shared spaces to determine user visibility of service instances

d92ffae

[#152035378] Signed-off-by: Alex Blease <ablease@pivotal.io>

Allow target space devs to view shared service instances

ec69e27

/v2/spaces/:guid/services endpoint now includes service instances that have been shared into the given space. [#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

Use &. operator instead of service_instance && service_instance.space

8e0cfde

[#152035378] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

Remove global auditors from read_permissions access

a72880e

[#152631507] Signed-off-by: Denise Yu <dyu@pivotal.io>

deniseyu force-pushed the pr-service-instance-sharing-view-shared-service-in-space branch from 140cbb7 to a72880e Compare November 7, 2017 14:01

matt-royal reviewed Nov 13, 2017

View reviewed changes

cf-gitbot added in progress and removed unscheduled labels Nov 13, 2017

matt-royal merged commit 35b8467 into cloudfoundry:master Nov 13, 2017

cf-gitbot added delivered and removed in progress labels Nov 13, 2017

cf-gitbot added accepted and removed delivered labels Nov 16, 2017

deniseyu deleted the pr-service-instance-sharing-view-shared-service-in-space branch November 28, 2017 16:49

Uh oh!

Conversation

jenspinney commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

First Important Note

Second Important Note

Summary of Changes:

PR

Uh oh!

cfdreddbot commented Oct 24, 2017

Uh oh!

cf-gitbot commented Oct 24, 2017

Uh oh!

cfdreddbot commented Oct 24, 2017

Uh oh!

tcdowney commented Oct 24, 2017

Uh oh!

tcdowney Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

jenspinney Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

Gerg commented Oct 31, 2017

Uh oh!

Gerg commented Oct 31, 2017

Uh oh!

jenspinney commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcdowney commented Nov 1, 2017

Uh oh!

Gerg commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deniseyu commented Nov 7, 2017

Uh oh!

matt-royal Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

jenspinney Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jenspinney commented Oct 24, 2017 •

edited

Loading

jenspinney commented Nov 1, 2017 •

edited

Loading

Gerg commented Nov 6, 2017 •

edited

Loading